Fix domain reference leaks

Besides two unlikely/rarely hit ones in x86 code, the main offender
was tmh_client_from_cli_id(), which didn't even have a counterpart
(albeit it had a comment correctly saying that it causes d->refcnt to
get incremented). Unfortunately(?) this required a bit of code
restructuring (as I needed to change the code anyway, I also fixed
a couple os missing bounds checks which would sooner or later be
reported as security vulnerabilities), so I would hope Dan could give
it his blessing before it gets applied.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 51193dd40bc2f4b74851f9b561811d76c2c27adc..98134b3b67ca92aed1475272727d64db11358ee8 100644 (file)
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -252,10 +252,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
         else
             len = __copy_from_user(buf, (void *)addr, len);
     }
-    else
+    else if ( dp )
     {
-        if ( dp && !dp->is_dying )   /* make sure guest is still there */
+        if ( !dp->is_dying )   /* make sure guest is still there */
             len= dbg_rw_guest_mem(addr, buf, len, dp, toaddr, pgd3);
+        put_domain(dp);
     }
 
     DBGP2("gmem:exit:len:$%d\n", len);

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 39c665d91438582b679e59bf57ac514363d1a40c..bc1134fd90d99f5081e0b4fae2639d6495a91c90 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3803,6 +3803,7 @@ int steal_page(
     struct domain *d, struct page_info *page, unsigned int memflags)
 {
     unsigned long x, y;
+    bool_t drop_dom_ref = 0;
 
     spin_lock(&d->page_alloc_lock);
 
@@ -3830,11 +3831,13 @@ int steal_page(
     } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
 
     /* Unlink from original owner. */
-    if ( !(memflags & MEMF_no_refcount) )
-        d->tot_pages--;
+    if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
+        drop_dom_ref = 1;
     page_list_del(page, &d->page_list);
 
     spin_unlock(&d->page_alloc_lock);
+    if ( unlikely(drop_dom_ref) )
+        put_domain(d);
     return 0;
 
  fail:

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index eed3192efb9e492c13c9212e43a4e5687f3302de..82075b6944cc39c303e2dcf1abccc6c380205b74 100644 (file)
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -912,14 +912,14 @@ static client_t *client_create(cli_id_t cli_id)
         return NULL;
     }
     memset(client,0,sizeof(client_t));
-    if ( (client->tmh = tmh_client_init()) == NULL )
+    if ( (client->tmh = tmh_client_init(cli_id)) == NULL )
     {
         printk("failed... can't allocate host-dependent part of client\n");
         if ( client )
             tmh_free_infra(client);
         return NULL;
     }
-    tmh_set_client_from_id(client,cli_id);
+    tmh_set_client_from_id(client, client->tmh, cli_id);
     client->cli_id = cli_id;
 #ifdef __i386__
     client->compress = 0;
@@ -1528,7 +1528,7 @@ static NOINLINE int do_tmem_destroy_pool(uint32_t pool_id)
 }
 
 static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
-                                     uint32_t this_pool_id, uint32_t flags,
+                                     uint32_t d_poolid, uint32_t flags,
                                      uint64_t uuid_lo, uint64_t uuid_hi)
 {
     client_t *client;
@@ -1540,19 +1540,13 @@ static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
     int specversion = (flags >> TMEM_POOL_VERSION_SHIFT)
          & TMEM_POOL_VERSION_MASK;
     pool_t *pool, *shpool;
-    int s_poolid, d_poolid, first_unused_s_poolid;
+    int s_poolid, first_unused_s_poolid;
     int i;
 
     if ( this_cli_id == CLI_ID_NULL )
-    {
-        client = tmh_client_from_current();
         cli_id = tmh_get_cli_id_from_current();
-    } else {
-        if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL)
-            return -EPERM;
+    else
         cli_id = this_cli_id;
-    }
-    ASSERT(client != NULL);
     printk("tmem: allocating %s-%s tmem pool for %s=%d...",
         persistent ? "persistent" : "ephemeral" ,
         shared ? "shared" : "private", cli_id_str, cli_id);
@@ -1573,19 +1567,24 @@ static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
     }
     if ( this_cli_id != CLI_ID_NULL )
     {
-        d_poolid = this_pool_id;
-        if ( client->pools[d_poolid] != NULL )
-            return -EPERM;
-        d_poolid = this_pool_id;
+        if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL
+             || d_poolid >= MAX_POOLS_PER_DOMAIN
+             || client->pools[d_poolid] != NULL )
+            goto fail;
     }
-    else for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN; d_poolid++ )
-        if ( client->pools[d_poolid] == NULL )
-            break;
-    if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
+    else
     {
-        printk("failed... no more pool slots available for this %s\n",
-            client_str);
-        goto fail;
+        client = tmh_client_from_current();
+        ASSERT(client != NULL);
+        for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN; d_poolid++ )
+            if ( client->pools[d_poolid] == NULL )
+                break;
+        if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
+        {
+            printk("failed... no more pool slots available for this %s\n",
+                   client_str);
+            goto fail;
+        }
     }
     if ( shared )
     {
@@ -1618,6 +1617,8 @@ static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
                     client->pools[d_poolid] = global_shared_pools[s_poolid];
                     shared_pool_join(global_shared_pools[s_poolid], client);
                     pool_free(pool);
+                    if ( this_cli_id != CLI_ID_NULL )
+                        tmh_client_put(client->tmh);
                     return d_poolid;
                 }
             }
@@ -1638,6 +1639,8 @@ static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
         }
     }
     client->pools[d_poolid] = pool;
+    if ( this_cli_id != CLI_ID_NULL )
+        tmh_client_put(client->tmh);
     list_add_tail(&pool->pool_list, &global_pool_list);
     pool->pool_id = d_poolid;
     pool->persistent = persistent;
@@ -1647,6 +1650,8 @@ static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
 
 fail:
     pool_free(pool);
+    if ( this_cli_id != CLI_ID_NULL )
+        tmh_client_put(client->tmh);
     return -EPERM;
 }
 
@@ -1672,6 +1677,7 @@ static int tmemc_freeze_pools(cli_id_t cli_id, int arg)
         if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
             return -1;
         client_freeze(client,freeze);
+        tmh_client_put(client->tmh);
         printk("tmem: all pools %s for %s=%d\n",s,cli_id_str,cli_id);
     }
     return 0;
@@ -1876,8 +1882,10 @@ static int tmemc_list(cli_id_t cli_id, tmem_cli_va_t buf, uint32_t len,
     }
     else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
         return -1;
-    else
+    else {
         off = tmemc_list_client(client, buf, 0, len, use_long);
+        tmh_client_put(client->tmh);
+    }
 
     return 0;
 }
@@ -1925,7 +1933,10 @@ static int tmemc_set_var(cli_id_t cli_id, uint32_t subop, uint32_t arg1)
     else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
         return -1;
     else
-            tmemc_set_var_one(client, subop, arg1);
+    {
+        tmemc_set_var_one(client, subop, arg1);
+        tmh_client_put(client->tmh);
+    }
     return 0;
 }
 
@@ -1941,6 +1952,8 @@ static NOINLINE int tmemc_shared_pool_auth(cli_id_t cli_id, uint64_t uuid_lo,
         return 1;
     }
     client = tmh_client_from_cli_id(cli_id);
+    if ( client == NULL )
+        return -EINVAL;
     for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++)
     {
         if ( (client->shared_auth_uuid[i][0] == uuid_lo) &&
@@ -1949,6 +1962,7 @@ static NOINLINE int tmemc_shared_pool_auth(cli_id_t cli_id, uint64_t uuid_lo,
             if ( auth == 0 )
                 client->shared_auth_uuid[i][0] =
                     client->shared_auth_uuid[i][1] = -1L;
+            tmh_client_put(client->tmh);
             return 1;
         }
         if ( (auth == 1) && (client->shared_auth_uuid[i][0] == -1L) &&
@@ -1956,11 +1970,15 @@ static NOINLINE int tmemc_shared_pool_auth(cli_id_t cli_id, uint64_t uuid_lo,
             free = i;
     }
     if ( auth == 0 )
+    {
+        tmh_client_put(client->tmh);
         return 0;
+    }
     if ( auth == 1 && free == -1 )
         return -ENOMEM;
     client->shared_auth_uuid[free][0] = uuid_lo;
     client->shared_auth_uuid[free][1] = uuid_hi;
+    tmh_client_put(client->tmh);
     return 1;
 }
 
@@ -1968,10 +1986,12 @@ static NOINLINE int tmemc_save_subop(int cli_id, uint32_t pool_id,
                         uint32_t subop, tmem_cli_va_t buf, uint32_t arg1)
 {
     client_t *client = tmh_client_from_cli_id(cli_id);
-    pool_t *pool =  (client == NULL) ? NULL : client->pools[pool_id];
+    pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+                   ? NULL : client->pools[pool_id];
     uint32_t p;
     uint64_t *uuid;
     pgp_t *pgp, *pgp2;
+    int rc = -1;
 
     switch(subop)
     {
@@ -1982,45 +2002,55 @@ static NOINLINE int tmemc_save_subop(int cli_id, uint32_t pool_id,
             if ( client->pools[p] != NULL )
                 break;
         if ( p == MAX_POOLS_PER_DOMAIN )
-            return 0;
+        {
+            rc = 0;
+            break;
+        }
         client->was_frozen = client->frozen;
         client->frozen = 1;
         if ( arg1 != 0 )
             client->live_migrating = 1;
-        return 1;
+        rc = 1;
+        break;
     case TMEMC_RESTORE_BEGIN:
-        ASSERT(client == NULL);
-        if ( (client = client_create(cli_id)) == NULL )
-            return -1;
-        return 1;
+        if ( client == NULL && (client = client_create(cli_id)) != NULL )
+            return 1;
+        break;
     case TMEMC_SAVE_GET_VERSION:
-        return TMEM_SPEC_VERSION;
+        rc = TMEM_SPEC_VERSION;
+        break;
     case TMEMC_SAVE_GET_MAXPOOLS:
-        return MAX_POOLS_PER_DOMAIN;
+        rc = MAX_POOLS_PER_DOMAIN;
+        break;
     case TMEMC_SAVE_GET_CLIENT_WEIGHT:
-        return client->weight == -1 ? -2 : client->weight;
+        rc = client->weight == -1 ? -2 : client->weight;
+        break;
     case TMEMC_SAVE_GET_CLIENT_CAP:
-        return client->cap == -1 ? -2 : client->cap;
+        rc = client->cap == -1 ? -2 : client->cap;
+        break;
     case TMEMC_SAVE_GET_CLIENT_FLAGS:
-        return (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
-               (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
+        rc = (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
+             (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
+        break;
     case TMEMC_SAVE_GET_POOL_FLAGS:
          if ( pool == NULL )
-             return -1;
-         return (pool->persistent ? TMEM_POOL_PERSIST : 0) |
-                (pool->shared ? TMEM_POOL_SHARED : 0) |
-                (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
+             break;
+         rc = (pool->persistent ? TMEM_POOL_PERSIST : 0) |
+              (pool->shared ? TMEM_POOL_SHARED : 0) |
+              (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
+        break;
     case TMEMC_SAVE_GET_POOL_NPAGES:
          if ( pool == NULL )
-             return -1;
-        return _atomic_read(pool->pgp_count);
+             break;
+        rc = _atomic_read(pool->pgp_count);
+        break;
     case TMEMC_SAVE_GET_POOL_UUID:
          if ( pool == NULL )
-             return -1;
+             break;
         uuid = (uint64_t *)buf.p;
         *uuid++ = pool->uuid[0];
         *uuid = pool->uuid[1];
-        return 0;
+        rc = 0;
     case TMEMC_SAVE_END:
         client->live_migrating = 0;
         if ( !list_empty(&client->persistent_invalidated_list) )
@@ -2028,27 +2058,34 @@ static NOINLINE int tmemc_save_subop(int cli_id, uint32_t pool_id,
               &client->persistent_invalidated_list, client_inv_pages)
                 pgp_free_from_inv_list(client,pgp);
         client->frozen = client->was_frozen;
-        return 0;
+        rc = 0;
     }
-    return -1;
+    if ( client )
+        tmh_client_put(client->tmh);
+    return rc;
 }
 
 static NOINLINE int tmemc_save_get_next_page(int cli_id, int pool_id,
                         tmem_cli_va_t buf, uint32_t bufsize)
 {
     client_t *client = tmh_client_from_cli_id(cli_id);
-    pool_t *pool =  (client == NULL) ? NULL : client->pools[pool_id];
+    pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+                   ? NULL : client->pools[pool_id];
     pgp_t *pgp;
     int ret = 0;
     struct tmem_handle *h;
     unsigned int pagesize = 1 << (pool->pageshift+12);
 
-    if ( pool == NULL )
-        return -1;
-    if ( is_ephemeral(pool) )
+    if ( pool == NULL || is_ephemeral(pool) )
+    {
+        tmh_client_put(client->tmh);
         return -1;
+    }
     if ( bufsize < pagesize + sizeof(struct tmem_handle) )
+    {
+        tmh_client_put(client->tmh);
         return -ENOMEM;
+    }
 
     tmem_spin_lock(&pers_lists_spinlock);
     if ( list_empty(&pool->persistent_page_list) )
@@ -2080,6 +2117,7 @@ static NOINLINE int tmemc_save_get_next_page(int cli_id, int pool_id,
 
 out:
     tmem_spin_unlock(&pers_lists_spinlock);
+    tmh_client_put(client->tmh);
     return ret;
 }
 
@@ -2094,7 +2132,10 @@ static NOINLINE int tmemc_save_get_next_inv(int cli_id, tmem_cli_va_t buf,
     if ( client == NULL )
         return 0;
     if ( bufsize < sizeof(struct tmem_handle) )
+    {
+        tmh_client_put(client->tmh);
         return 0;
+    }
     tmem_spin_lock(&pers_lists_spinlock);
     if ( list_empty(&client->persistent_invalidated_list) )
         goto out;
@@ -2121,6 +2162,7 @@ static NOINLINE int tmemc_save_get_next_inv(int cli_id, tmem_cli_va_t buf,
     ret = 1;
 out:
     tmem_spin_unlock(&pers_lists_spinlock);
+    tmh_client_put(client->tmh);
     return ret;
 }
 
@@ -2128,22 +2170,26 @@ static int tmemc_restore_put_page(int cli_id, int pool_id, uint64_t oid,
                       uint32_t index, tmem_cli_va_t buf, uint32_t bufsize)
 {
     client_t *client = tmh_client_from_cli_id(cli_id);
-    pool_t *pool =  (client == NULL) ? NULL : client->pools[pool_id];
+    pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+                   ? NULL : client->pools[pool_id];
+    int rc = pool ? do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p) : -1;
 
-    if ( pool == NULL )
-        return -1;
-    return do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p);
+    if ( client )
+        tmh_client_put(client->tmh);
+    return rc;
 }
 
 static int tmemc_restore_flush_page(int cli_id, int pool_id, uint64_t oid,
                         uint32_t index)
 {
     client_t *client = tmh_client_from_cli_id(cli_id);
-    pool_t *pool =  (client == NULL) ? NULL : client->pools[pool_id];
+    pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+                   ? NULL : client->pools[pool_id];
+    int rc = pool ? do_tmem_flush_page(pool, oid, index) : -1;
 
-    if ( pool == NULL )
-        return -1;
-    return do_tmem_flush_page(pool, oid, index);
+    if ( client )
+        tmh_client_put(client->tmh);
+    return rc;
 }
 
 static NOINLINE int do_tmem_control(struct tmem_op *op)

diff --git a/xen/common/tmem_xen.c b/xen/common/tmem_xen.c
index 950c9c03da1bcdc34985d7ab23ce4b96b631c487..b0d572a4ffdf29e92d0df88b50686e1eaedf35ac 100644 (file)
--- a/xen/common/tmem_xen.c
+++ b/xen/common/tmem_xen.c
@@ -286,17 +286,16 @@ static void tmh_persistent_pool_page_put(void *page_va)
 
 /******************  XEN-SPECIFIC CLIENT HANDLING ********************/
 
-EXPORT tmh_client_t *tmh_client_init(void)
+EXPORT tmh_client_t *tmh_client_init(cli_id_t cli_id)
 {
     tmh_client_t *tmh;
     char name[5];
-    domid_t domid = current->domain->domain_id;
     int i, shift;
 
     if ( (tmh = xmalloc(tmh_client_t)) == NULL )
         return NULL;
     for (i = 0, shift = 12; i < 4; shift -=4, i++)
-        name[i] = (((unsigned short)domid >> shift) & 0xf) + '0';
+        name[i] = (((unsigned short)cli_id >> shift) & 0xf) + '0';
     name[4] = '\0';
 #ifndef __i386__
     tmh->persistent_pool = xmem_pool_create(name, tmh_persistent_pool_page_get,
@@ -307,7 +306,6 @@ EXPORT tmh_client_t *tmh_client_init(void)
         return NULL;
     }
 #endif
-    tmh->domain = current->domain;
     return tmh;
 }
 
@@ -317,6 +315,7 @@ EXPORT void tmh_client_destroy(tmh_client_t *tmh)
     xmem_pool_destroy(tmh->persistent_pool);
 #endif
     put_domain(tmh->domain);
+    tmh->domain = NULL;
 }
 
 /******************  XEN-SPECIFIC HOST INITIALIZATION ********************/

diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h
index 394251dfef7393d8d459913e5b8726571f2cd845..56be37e950788c56359548601b729a21a0192cd2 100644 (file)
--- a/xen/include/xen/tmem_xen.h
+++ b/xen/include/xen/tmem_xen.h
@@ -43,8 +43,6 @@ extern rwlock_t tmem_rwlock;
 
 extern void tmh_copy_page(char *to, char*from);
 extern int tmh_init(void);
-extern tmh_client_t *tmh_client_init(void);
-extern void tmh_client_destroy(tmh_client_t *);
 #define tmh_hash hash_long
 
 extern void tmh_release_avail_pages_to_host(void);
@@ -281,6 +279,9 @@ typedef domid_t cli_id_t;
 typedef struct domain tmh_cli_ptr_t;
 typedef struct page_info pfp_t;
 
+extern tmh_client_t *tmh_client_init(cli_id_t);
+extern void tmh_client_destroy(tmh_client_t *);
+
 /* this appears to be unreliable when a domain is being shut down */
 static inline struct client *tmh_client_from_cli_id(cli_id_t cli_id)
 {
@@ -290,6 +291,11 @@ static inline struct client *tmh_client_from_cli_id(cli_id_t cli_id)
     return (struct client *)(d->tmem);
 }
 
+static inline void tmh_client_put(tmh_client_t *tmh)
+{
+    put_domain(tmh->domain);
+}
+
 static inline struct client *tmh_client_from_current(void)
 {
     return (struct client *)(current->domain->tmem);
@@ -307,10 +313,12 @@ static inline tmh_cli_ptr_t *tmh_get_cli_ptr_from_current(void)
     return current->domain;
 }
 
-static inline void tmh_set_client_from_id(struct client *client,cli_id_t cli_id)
+static inline void tmh_set_client_from_id(struct client *client,
+                                          tmh_client_t *tmh, cli_id_t cli_id)
 {
     struct domain *d = get_domain_by_id(cli_id);
     d->tmem = client;
+    tmh->domain = d;
 }
 
 static inline bool_t tmh_current_is_privileged(void)